Bitcoin hashes no default features #410

tcharding · 2022-03-01T16:31:16Z

Currently we use default features for the bitcoin_hashes dependency, doing so breaks the no-std feature in rust-bitcoin because std is part of bitcoin_hashes default feature set.

Disable bitcoin_hashes default features, no changes to rust-bitcoin are require after this change since we manually turn on std and alloc as part of the std/no-std features of rust-bitcoin.

For other users of rust-secp256k1 this is a breaking change but is unlikely to cause too much bother because std is so commonly used.

This PR resolves an open issue in rust-bitcoin, see issue for discussion.

Mirror the whitespacing in `rust-bitcoin` by doing: - Only use single line of whitespace between sections - Separate optional dependencies from non-optional ones

Currently we use default features for the `bitcoin_hashes` dependency, doing so breaks the `no-std` feature in `rust-bitcoin` because `std` is part of `bitcoin_hashes` default feature set. Disable `bitcoin_hashes` default features, no changes to `rust-bitcoin` are require after this change since we manually turn on `std` and `alloc` as part of the `std`/`no-std` features of `rust-bitcoin`. For other users of `rust-secp256k1` this is a breaking change but is unlikely to cause too much bother because `std` is so commonly used.

Kixunil · 2022-03-01T16:57:10Z

Looks good but watch out for compatibility hazards. We should at least test the crate with features std, bitcoin_hashes but without bitcoin_hashes/std. (Similarly with other crates such as serde.)

tcharding · 2022-03-01T20:11:28Z

IIUC there is now no way of turning on bitcoin_hashes/std while running test.sh. Do you mean we should add more tests for certain combinations of multiple features? I do not personally know which combinations would be useful to test. (We already have one useless combination, rand and rand-std :)

We do all combinations in bitcoin_hashes but that explodes here because we have so many features.

Kixunil · 2022-03-01T20:25:06Z

Do you mean we should add more tests for certain combinations of multiple features?

I believe so.

I do not personally know which combinations would be useful to test.

Each external crate with optional alloc + alloc; each external crate with optional std + std. Maybe all crates can be tested at once. The difference between all features is special features (e.g. rand-std) are not enabled.

Now that I had to explained it it occurred to me bitcoin_hashes can not be causing problems because it doesn't have _std feature and --all-features succeeds.

thomaseizinger · 2022-03-01T21:54:29Z

If this PR compiles without problems, does that mean we don't use any std functionality of bitcoin_hashes ever in this crate?

Kixunil · 2022-03-01T22:37:35Z

@thomaseizinger yes, I'm curious, so will check why.

Update: checked and it's all very sensible. We're not missing anything. :)

tcharding · 2022-03-02T09:14:42Z

I've added an additional patch that adds features serde-std and bitcoin-hashes-std and improves the test matrix in test.sh. Please see the commit message for full explanation. Note that 'alloc' combos are not tested, I don't see any way to do so from test.sh and as we explored in another PR we don't want to have features for all combinations of each dependency/feature.

Kixunil · 2022-03-02T09:51:16Z

Cargo.toml

 rand-std = ["rand/std"]
+serde-std = ["serde/std"]


I don't think proactively adding these feature is very valuable because forward-compatibility is not affected. They are only helpful for convenience. If you took inspiration from ln-types then I'm sorry, I only did proper analysis now and I'll probably remove unused features from ln-types.

Analysis:

If secp256k1 will add new stuff in the future depending on bitcoin_hashes/std the feature can be added at that time. There's no backcompat-related harm in adding default-off feature later.

If bitcoin_hashes changes existing APIs to start requiring std it's a breaking change and it'll have to bump major version (or minor when major is 0).

Bumping major version of a publicly-visible dependency is a breaking change - in our case 100% every time because we reexport it.

Conclusion: adding the feature later is backwards compatible, adding it sooner is only a helper. YAGNI? Annoyance around adding can be solved by better tooling and the solution will be better. I will open an issue at cargo add

I mentioned this in cargo add feedback since it probably should be the default: https://internals.rust-lang.org/t/feedback-on-cargo-add-before-its-merged/16024/58?u=kixunil

Thanks for the explanation. I've removed the serde-std feature as suggested and left just what is needed to solve the problem we have (bitcoin-hashes-std).

YAGNI comment was particularly interesting. I've found myself recently not exactly sure where to draw the line on what we should include and what we shouldn't. Please keep reminding me of this if I lean to much towards adding YAGNI code. Thanks.

Kixunil · 2022-03-02T09:55:39Z

contrib/test.sh

+    # Test features that will likely only be enabled along with `std`.
+    cargo test --all --features="rand-std"
+    cargo test --all --features="bitcoin-hashes-std"
+    cargo test --all --features="serde-std"


These are correct but a bit confusing. How about having another variable with these and a comment explaining why they are separate?

Good idea, done as suggested.

apoelstra

ACK 3697003

I generally agree wiht @Kixunil's comments though I don't feel too strongly about them. Will hold off merging til they are resolved.

Currently we use 'no default features' for the `bitcoin_hashes` dependency. Doing so means that if users want the `std` feature they need to explicitly add a `bitcoin_hashes` dependency even though we re-export `bitcoin_hashes` as `hashes`. This means that in the common case the re-export is pointless. As an example, `rust-bitcoin` unnecessarily requires an explicit dependency on `bitcoin_hashes`. Add `bitcoin-hashes-std` feature so that users do not need an explicit dependency in the common use case. Change the test matrix to only test '*-std' features when 'std' is enabled since enabling one without the other is illogical. Please note, this replaces the test run of feature 'std'+'rand'+'rand-std' with just 'std'+'rand-std' because enabling 'rand-std' enables 'rand' so the explicit additional feature is redundant.

tcharding · 2022-03-07T23:49:54Z

Force push includes changes only to the final patch to remove the serde-std feature.

apoelstra

ACK 6bcf3ea

tcharding added 2 commits March 1, 2022 16:22

Improve manifest whitespace

b6f169f

Mirror the whitespacing in `rust-bitcoin` by doing: - Only use single line of whitespace between sections - Separate optional dependencies from non-optional ones

tcharding mentioned this pull request Mar 1, 2022

"bitcoin_hashes" feature in rust-secp seems to break CI rust-bitcoin/rust-bitcoin#830

Closed

kafaichoi mentioned this pull request Mar 2, 2022

infallible conversions from hashes to secp256k1 message rust-bitcoin/rust-bitcoin#851

Closed

Kixunil reviewed Mar 2, 2022

View reviewed changes

apoelstra previously approved these changes Mar 2, 2022

View reviewed changes

apoelstra mentioned this pull request Mar 2, 2022

update libsecp to latest ersion; bump major version number #384

Merged

tcharding dismissed apoelstra’s stale review via 6bcf3ea March 7, 2022 23:45

tcharding force-pushed the bitcoin_hashes-no-default-features branch from 3697003 to 6bcf3ea Compare March 7, 2022 23:45

apoelstra approved these changes Mar 8, 2022

View reviewed changes

apoelstra merged commit d4d74bf into rust-bitcoin:master Mar 8, 2022

tcharding deleted the bitcoin_hashes-no-default-features branch March 8, 2022 21:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bitcoin hashes no default features #410

Bitcoin hashes no default features #410

tcharding commented Mar 1, 2022 •

edited

Loading

Kixunil commented Mar 1, 2022

tcharding commented Mar 1, 2022

Kixunil commented Mar 1, 2022

thomaseizinger commented Mar 1, 2022

Kixunil commented Mar 1, 2022 •

edited

Loading

tcharding commented Mar 2, 2022

Kixunil Mar 2, 2022

Kixunil Mar 2, 2022

tcharding Mar 7, 2022

tcharding Mar 7, 2022

Kixunil Mar 2, 2022

tcharding Mar 7, 2022

apoelstra left a comment

tcharding commented Mar 7, 2022

apoelstra left a comment •

edited

Loading

Bitcoin hashes no default features #410

Bitcoin hashes no default features #410

Conversation

tcharding commented Mar 1, 2022 • edited Loading

Kixunil commented Mar 1, 2022

tcharding commented Mar 1, 2022

Kixunil commented Mar 1, 2022

thomaseizinger commented Mar 1, 2022

Kixunil commented Mar 1, 2022 • edited Loading

tcharding commented Mar 2, 2022

Kixunil Mar 2, 2022

Choose a reason for hiding this comment

Kixunil Mar 2, 2022

Choose a reason for hiding this comment

tcharding Mar 7, 2022

Choose a reason for hiding this comment

tcharding Mar 7, 2022

Choose a reason for hiding this comment

Kixunil Mar 2, 2022

Choose a reason for hiding this comment

tcharding Mar 7, 2022

Choose a reason for hiding this comment

apoelstra left a comment

Choose a reason for hiding this comment

tcharding commented Mar 7, 2022

apoelstra left a comment • edited Loading

Choose a reason for hiding this comment

tcharding commented Mar 1, 2022 •

edited

Loading

Kixunil commented Mar 1, 2022 •

edited

Loading

apoelstra left a comment •

edited

Loading